Detect unspecified thirdbody collision partners #1015

ischoegl · 2021-04-19T12:25:46Z

Changes proposed in this pull request

This PR is the second half of a fix for #967 (with the first half being taken care of by #968). I decided to post this as a dedicated PR instead of attaching the commits to #968, as review policies weren't clear, and this addition doesn't depend on the solution proposed in #968.

I believe the question on how to treat concentration has been settled in discussions in #967 and #968, but a process to convert reactions that explicitly specify a collision partner was still missing (per @decaluwe's suggestion). As an alternative to explicitly relabeling all affected reactions, it is possible to leave the existing syntax

- equation: H + 2 O2 <=> HO2 + O2
  rate-constant: {A: 2.08e+19, b: -1.24, Ea: 0.0}

and use simple rules to identify this reaction as a three-body reaction in a pre-processing step (similar to what is already done for the detection of electrochemical reactions). I.e. O2 would be detected as the collision partner, and treated as if one would manually specify

- equation: H + O2 + M <=> HO2 + M
  rate-constant: {A: 2.08e+19, b: -1.24, Ea: 0.0}
  type: three-body
  default-efficiency: 0
  efficiencies: {O2: 1.0}

Specifically, I am suggesting the following four rules for converting an elementary to a three-body reaction:

one species has to occur on both sides
reaction type must not be specified
integer stoichiometric coefficients for all participating species
one side has to have exactly 3 participating species to qualify

If applicable, fill in the issue number this pull request is fixing

Fixes #967 (as long as #968 is merged also)

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

ischoegl · 2021-04-19T22:46:42Z

@speth, @decaluwe and @rwest ... after making this its own PR - and adding unit tests - I believe this is ready for review and/or comments. If the approach is acceptable and finds approval, it should probably be merged at the same time as #968 so #967 can be resolved.

rwest

Looks OK to me! Thanks Ingmar

Per discussion in Cantera#967, species-specific third-body reactions should use molar concentrations for collision partner concentrations instead of activity concentrations to be consistent with three-body reactions with generic collision partners.

A reaction is identified as a three-body reaction if the following criteria are satisfied: * reaction type must not be specified * exactly one species has to occur on both reactant and product side * all stoichiometric coefficients have integer values * one side has to have exactly 3 participating species

* test-clib: species-specific three-body reaction are reformatted * test-cxx/test-f90: reformatted species-specific three-body reactions; change of spurious net reaction rates, which should be zero * test-f77-ctlib: change of spurious net production rate for AR (gri30)

ischoegl · 2021-04-20T21:45:26Z

@decaluwe ... I saw that you merged #968 (which is the companion PR). I resolved the merge conflict created by Blowers-Masel, so this is ready for review again.

speth

This looks pretty good to me, @ischoegl. All of my comments are on fairly minor items. Beyond the in-line comments, there are just a couple of other things:

Can you add a test for a reaction created via XML, given that you implemented this logic for both YAML and CTI/XML derived reactions?
Have you thought about how these reactions should be serialized? Right now, they are converted to three-body reactions with the default-efficiency and efficiencies fields set as one might expect. I guess the question is whether or not that extra information should be deleted.

Lastly, I just wanted to note that I like that this ends up moving the third body to the end of the reactant / product strings for these reactions.

include/cantera/kinetics/Reaction.h

interfaces/cython/cantera/test/test_kinetics.py

interfaces/cython/cantera/test/test_reaction.py

src/kinetics/ReactionFactory.cpp

src/kinetics/Kinetics.cpp

src/kinetics/ReactionFactory.cpp

test_problems/fortran/f77_ctlib_blessed.txt

src/kinetics/Reaction.cpp

ischoegl · 2021-04-21T19:12:48Z

@speth ... thanks for your review. Hadn't thought about serialization as this is still a brand-new addition, but your point is well taken. I had to change duplicate checks but will look at this again.

I just wanted to note that I like that this ends up moving the third body to the end of the reactant / product strings for these reactions.

Likewise. Have to admit that this was an unintended 'feature', and would probably have fought to keep the new behavior (as it is both more instructive and easier to implement).

PS: Regarding XML tests, those are implicitly included by test_convert.py.

ischoegl · 2021-04-22T20:52:08Z

@speth (and @bryanwweber) ... I think this should take care of the review comments. I did not feel the need to implement XML unit tests, as those are (implicitly) covered by test_convert.py: it is already verified that equation types created from CTI and YAML correspond.

I did, however, implement a shortened serialization output, which I had not thought about.

src/kinetics/Reaction.cpp

Also, address review comments

Three-body reactions with explictly declared collision partners do not need to declare additional fields, i.e. 'type', 'default-efficiency', and 'efficiencies'.

ischoegl · 2021-04-27T00:21:27Z

Thanks @speth!

ischoegl changed the title ~~Detect thirdbody collision partners~~ Detect unspecified thirdbody collision partners Apr 19, 2021

ischoegl added the Input Input parsing and conversion (for example, ck2yaml) label Apr 19, 2021

ischoegl force-pushed the fix-967-detect-thirdbodies branch from 5b2142b to 8b84969 Compare April 19, 2021 22:30

ischoegl requested a review from a team April 19, 2021 22:42

rwest reviewed Apr 20, 2021

View reviewed changes

ischoegl added 5 commits April 20, 2021 16:27

[Kinetics] Catch undetected third bodies in calculateRateCoeffUnits

3a459cb

[CI] unit tests for detection of implicit three-body reactions

33f05f8

ischoegl force-pushed the fix-967-detect-thirdbodies branch from 8b84969 to 33f05f8 Compare April 20, 2021 21:30

speth requested changes Apr 21, 2021

View reviewed changes

ischoegl requested a review from speth April 22, 2021 21:05

speth reviewed Apr 22, 2021

View reviewed changes

src/kinetics/Reaction.cpp Outdated Show resolved Hide resolved

ischoegl added 2 commits April 22, 2021 20:01

[Kinetics] Update duplicate check for undeclared three-body reactions

0ac9e8b

Also, address review comments

[Kinetics] Update serialization of three-body reactions

f61f85c

Three-body reactions with explictly declared collision partners do not need to declare additional fields, i.e. 'type', 'default-efficiency', and 'efficiencies'.

ischoegl force-pushed the fix-967-detect-thirdbodies branch from 188a1a0 to f61f85c Compare April 23, 2021 01:01

ischoegl requested a review from speth April 23, 2021 22:49

speth approved these changes Apr 27, 2021

View reviewed changes

speth merged commit b753dd5 into Cantera:main Apr 27, 2021

speth mentioned this pull request Apr 27, 2021

Skipping undeclared species does not work with specific third body detection #1024

Closed

ischoegl deleted the fix-967-detect-thirdbodies branch June 22, 2021 21:28

speth mentioned this pull request Oct 19, 2021

Yaml file backwards compatibility #1092

Closed

ischoegl mentioned this pull request Feb 10, 2022

Undeclared duplicate reaction error for generic third body reaction matching the form of another reaction #1193

Closed

ischoegl mentioned this pull request Mar 15, 2022

Two-temperature-plasma incompatible with third-body notation #1220

Closed

ischoegl mentioned this pull request Apr 25, 2022

Reaction parsed (incorrectly?) as three-body reaction using different versions of cantera #1251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect unspecified thirdbody collision partners #1015

Detect unspecified thirdbody collision partners #1015

ischoegl commented Apr 19, 2021 •

edited

Loading

ischoegl commented Apr 19, 2021

rwest left a comment

ischoegl commented Apr 20, 2021

speth left a comment

ischoegl commented Apr 21, 2021 •

edited

Loading

ischoegl commented Apr 22, 2021 •

edited

Loading

ischoegl commented Apr 27, 2021

Detect unspecified thirdbody collision partners #1015

Detect unspecified thirdbody collision partners #1015

Conversation

ischoegl commented Apr 19, 2021 • edited Loading

ischoegl commented Apr 19, 2021

rwest left a comment

Choose a reason for hiding this comment

ischoegl commented Apr 20, 2021

speth left a comment

Choose a reason for hiding this comment

ischoegl commented Apr 21, 2021 • edited Loading

ischoegl commented Apr 22, 2021 • edited Loading

ischoegl commented Apr 27, 2021

ischoegl commented Apr 19, 2021 •

edited

Loading

ischoegl commented Apr 21, 2021 •

edited

Loading

ischoegl commented Apr 22, 2021 •

edited

Loading